Make REST catalog namespace separator configurable #2826
Conversation
|
@geruh thanks so much for your review! I've addressed your comments |
|
i was reviewing this and got distracted, a couple of times 😆 |
kevinjqliu
left a comment
kevinjqliu
left a comment
There was a problem hiding this comment.
LGTM! Thanks for adding this feature
Could you add an integration test against the iceberg-rest-fixture? Currently http://localhost:8181/v1/config returns
"overrides": {
"namespace-separator": "%2E"
},
| return {"namespace": NAMESPACE_SEPARATOR.join(identifier.namespace.root), kind.value: identifier.name} | ||
| return { | ||
| "namespace": self._encode_namespace_path(tuple(identifier.namespace.root)), | ||
| kind.value: quote(identifier.name, safe=""), |
There was a problem hiding this comment.
curious why quote is used here
There was a problem hiding this comment.
What if the namespace separator has a '/' in it? We need to make sure that it isn't interpreted as part of the path.
Added a comment to make this clearer.
There was a problem hiding this comment.
nit: should we make this a helper function? i can see a scenario where someone will forget to use quote
Fokko
left a comment
Fokko
left a comment
There was a problem hiding this comment.
Nice one @rambleraptor
I agree with @kevinjqliu's comments, but apart from that this looks good to me. Thanks for adding this 👍
rambleraptor
force-pushed
the
namespace_configure_rest
branch
from
20cea63 to
dac44f4
Compare
The REST spec currently uses %1F as the UTF-8 encoded namespace separator for multi-part namespaces.
rambleraptor
force-pushed
the
namespace_configure_rest
branch
from
dac44f4 to
2c616fc
Compare
|
@kevinjqliu @Fokko ready to merge! |
| # fallback in order to work with older rest catalog implementations | ||
| if Capability.V1_NAMESPACE_EXISTS not in self._supported_endpoints: | ||
| try: | ||
| self.load_namespace_properties(namespace_tuple) | ||
| return True | ||
| except NoSuchNamespaceError: | ||
| return False |
There was a problem hiding this comment.
is this intentional? i think we still need this
| return {"namespace": NAMESPACE_SEPARATOR.join(identifier.namespace.root), kind.value: identifier.name} | ||
| return { | ||
| "namespace": self._encode_namespace_path(tuple(identifier.namespace.root)), | ||
| kind.value: quote(identifier.name, safe=""), |
There was a problem hiding this comment.
nit: should we make this a helper function? i can see a scenario where someone will forget to use quote
4 participants
Closes #1183
This adds a configurable namespace separator in the REST Catalog.
Rationale for this change
Certain implementations expect a different namespace separator.
Are these changes tested?
Tests included.
Are there any user-facing changes?